fix(gemini): apply timeout after client_params merge and handle HttpOptions object by ayushh0110 · Pull Request #7629 · agno-agi/agno

ayushh0110 · 2026-04-22T17:27:06Z

Description

Fixes two bugs in Gemini.get_client() that caused self.timeout to be silently ignored when client_params is provided.

Fixes #7599

Root Cause

Bug 1 — Wrong order: timeout was applied before `client_params.update()`

# BEFORE (broken)
if self.timeout is not None:
    http_options = client_params.get("http_options", {})
    if isinstance(http_options, dict):
        http_options["timeout"] = int(self.timeout * 1000)
        client_params["http_options"] = http_options  # ← set here

if self.client_params:
    client_params.update(self.client_params)  # ← overwrites http_options entirely

Any http_options key inside client_params would overwrite the injected timeout, causing requests to fall back to the underlying client default (e.g. 120 seconds) regardless of what the user set.

Bug 2 — Type guard: `HttpOptions` object silently skipped

# BEFORE (broken)
if isinstance(http_options, dict):   # fails for HttpOptions(...) objects → timeout skipped
    http_options["timeout"] = int(self.timeout * 1000)

When a user passes http_options=HttpOptions(some_option="value") inside client_params, the isinstance(dict) guard evaluates to False and timeout injection is silently skipped with no warning or error.

Fix

# AFTER (fixed)
if self.client_params:
    client_params.update(self.client_params)  # merge first

if self.timeout is not None:
    http_options = client_params.get("http_options", {})
    if hasattr(http_options, "model_dump"):
        # HttpOptions (or any pydantic model) → normalise to plain dict
        http_options = http_options.model_dump(exclude_none=True)
    elif not isinstance(http_options, dict):
        http_options = {}
    # Only inject timeout when the caller has not already set one
    if "timeout" not in http_options:
        http_options["timeout"] = int(self.timeout * 1000)
    client_params["http_options"] = http_options

Changes:

Move timeout injection after client_params.update() so all user params are fully merged first.
Normalize http_options to a plain dict via model_dump(exclude_none=True) when it is an HttpOptions / pydantic object, preserving any other options the user set.
Only inject self.timeout when the caller has not already set a timeout key, so explicit user overrides are respected.

Tests

Added 3 regression tests to the existing TestGeminiTimeout class in tests/unit/models/google/test_gemini.py:

Test	What it covers
`test_timeout_survives_client_params_update`	Bug 1 — timeout is present even when `client_params` carries unrelated keys
`test_timeout_with_http_options_object_in_client_params`	Bug 2 — `HttpOptions`-like object is normalized; timeout is injected and existing options preserved
`test_explicit_timeout_in_client_params_http_options_takes_precedence`	User's explicit `timeout` inside `client_params` http_options is not overwritten

All 7 tests in TestGeminiTimeout pass.

Type of Change

Bug fix (non-breaking change which fixes an issue)
Tests added / updated

…ptions object Two bugs in Gemini.get_client() caused self.timeout to be silently ignored: 1. Ordering bug: timeout was injected into http_options BEFORE client_params.update(self.client_params) was called. Any http_options key inside client_params therefore overwrote the injected timeout. 2. Type bug: the isinstance(http_options, dict) guard silently skipped timeout injection when the user passed an HttpOptions object (e.g. HttpOptions(some_option='value')) inside client_params. The object branch was simply not handled. Fix: - Move the timeout injection block AFTER client_params.update() so client_params are fully merged before the timeout is applied. - Normalise http_options to a plain dict before merging: call model_dump(exclude_none=True) for pydantic/HttpOptions objects and fall back to {} for any other non-dict type. - Only inject self.timeout when the caller has NOT already set a timeout key, preserving explicit user overrides. Fixes agno-agi#7599

VANDRANKI

Two bugs fixed correctly in a single PR.

Bug 1 (order): client_params.update() was called after the timeout was injected, so any http_options key inside client_params would overwrite the just-set timeout. Moving the update() call before the timeout injection is the right fix.

Bug 2 (type): isinstance(http_options, dict) returned False for HttpOptions objects, silently skipping the timeout injection entirely. Normalizing via model_dump(exclude_none=True) is the right approach.

if 'timeout' not in http_options is a good addition - if the caller explicitly set a timeout in their http_options, this PR correctly preserves it rather than overwriting it. The deleted test test_timeout_does_not_override_client_params_http_options was testing wrong behavior; its replacement test_explicit_timeout_in_client_params_http_options_takes_precedence tests the correct behavior.

Minor: hasattr(http_options, 'model_dump') is duck typing that would match any object with a model_dump method. The more precise check would be isinstance(http_options, BaseModel) (importing from pydantic), or even isinstance(http_options, HttpOptions) if google.genai.types.HttpOptions is importable here. That said, duck typing is a reasonable tradeoff if the import adds overhead.

Also minor: the elif not isinstance(http_options, dict): http_options = {} branch silently drops any unrecognized http_options object. A log_warning here would help users debug unexpected configurations.

The three regression tests directly pin both bugs. LGTM.

ayushh0110 · 2026-04-23T09:01:27Z

Thanks for the review @VANDRANKI — addressed the minor feedback:

log_warning added (e712285): The elif not isinstance(http_options, dict) fallback now logs a warning with the type name before falling back to an empty dict, so users can debug unexpected configurations.

Re: duck typing — keeping hasattr(http_options, 'model_dump') as-is. While BaseModel is already imported, HttpOptions inherits from google.genai._common.BaseModel (which itself subclasses pydantic's BaseModel), so isinstance would work today. But the duck-typed check is more resilient to future google-genai refactors where the class hierarchy might change. As you noted, reasonable tradeoff.

VANDRANKI · 2026-04-23T21:28:31Z

The log_warning addition in e712285 is the right call. Users who pass an unrecognized http_options type would previously see the timeout silently not applied with no indication of why - the warning with the type name gives them an immediate actionable signal.

On the duck-typing rationale: agreed. hasattr(http_options, 'model_dump') is more resilient than isinstance(http_options, HttpOptions) here because it doesn't couple the check to the current class hierarchy in google-genai. If a future refactor moves HttpOptions to a different base or introduces a compatible alternative, the duck-typed check still works without needing an import or an update to this branch.

LGTM.

…-object

ayushh0110 · 2026-04-25T16:09:34Z

Hi @ashpreetbedi — this fix has community LGTM, tests passing, and addresses the timeout issue in #7599. Would appreciate a review when you get a chance!

VANDRANKI

The fix correctly addresses both sub-bugs. A few observations.

Bug 1 (ordering): correct fix

Moving before the timeout injection is the right approach. The old order let clobber the injected entirely.

Bug 2 (HttpOptions object): correct fix

to detect a Pydantic model and then to flatten it into a plain dict before merging is clean. One edge case: if is not a Pydantic model in a future SDK version but still has a method with different semantics, this could break. A more defensive check would be with , but the current duck-typing approach is pragmatic and fine for now.

Deleted test concern

The removed test would still pass under the new logic: when supplies , after /dict-check the dict already contains , so the guard correctly skips injection and the effective timeout remains 60000 ms. The test was deleted unnecessarily. Restoring it documents the caller-explicit-timeout-wins precedence rule. Not blocking, but worth adding back as a positive assertion.

New test coverage

and together cover the two regression scenarios directly. Good.

LGTM. Ready to merge once the deleted test is optionally restored.

…tp_options Restores the deleted test as suggested by reviewer. The test still passes under the new logic and documents the caller-explicit-timeout-wins precedence rule as a positive assertion.

ayushh0110 · 2026-04-29T22:21:29Z

Thanks @VANDRANKI — restored test_timeout_does_not_override_client_params_http_options (
All 8 tests in TestGeminiTimeout pass.
@ashpreetbedi — friendly bump! PR has approval, all feedback addressed, and tests green. Would appreciate a merge when you get a chance.

ayushh0110 requested a review from a team as a code owner April 22, 2026 17:27

github-actions Bot added the first-time-contributor label Apr 22, 2026

ayushh0110 mentioned this pull request Apr 22, 2026

[Bug] Gemini timeout is ignored when http_options is set via client_params #7599

Open

github-actions Bot mentioned this pull request Apr 22, 2026

fix(gemini): apply timeout after client_params merge and support HttpOptions objects #7631

Closed

VANDRANKI reviewed Apr 23, 2026

View reviewed changes

fix(gemini): log warning when unrecognized http_options type is dropped

e712285

ayushh0110 added 2 commits April 24, 2026 03:40

Merge branch 'main' into fix/gemini-timeout-ignored-with-http-options…

fae0c5c

…-object

Merge branch 'main' into fix/gemini-timeout-ignored-with-http-options…

2b8ee4d

…-object

VANDRANKI approved these changes Apr 27, 2026

View reviewed changes

test(gemini): restore test_timeout_does_not_override_client_params_ht…

f5a40bf

…tp_options Restores the deleted test as suggested by reviewer. The test still passes under the new logic and documents the caller-explicit-timeout-wins precedence rule as a positive assertion.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(gemini): apply timeout after client_params merge and handle HttpOptions object#7629

fix(gemini): apply timeout after client_params merge and handle HttpOptions object#7629
ayushh0110 wants to merge 5 commits intoagno-agi:mainfrom
ayushh0110:fix/gemini-timeout-ignored-with-http-options-object

ayushh0110 commented Apr 22, 2026

Uh oh!

VANDRANKI left a comment

Uh oh!

ayushh0110 commented Apr 23, 2026

Uh oh!

VANDRANKI commented Apr 23, 2026

Uh oh!

ayushh0110 commented Apr 25, 2026 •

edited

Loading

Uh oh!

VANDRANKI left a comment

Uh oh!

ayushh0110 commented Apr 29, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

ayushh0110 commented Apr 22, 2026

Description

Root Cause

Bug 1 — Wrong order: timeout was applied before client_params.update()

Bug 2 — Type guard: HttpOptions object silently skipped

Fix

Tests

Type of Change

Uh oh!

VANDRANKI left a comment

Choose a reason for hiding this comment

Uh oh!

ayushh0110 commented Apr 23, 2026

Uh oh!

VANDRANKI commented Apr 23, 2026

Uh oh!

ayushh0110 commented Apr 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

VANDRANKI left a comment

Choose a reason for hiding this comment

Uh oh!

ayushh0110 commented Apr 29, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Bug 1 — Wrong order: timeout was applied before `client_params.update()`

Bug 2 — Type guard: `HttpOptions` object silently skipped

ayushh0110 commented Apr 25, 2026 •

edited

Loading